Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use IAction instead of Action for search item action #164844

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Oct 28, 2022

For actions that are static, it is better to use IAction instead of Action. This avoids creating an extra disposable, emitter, and event listener (when used with an action bar). It also means you don't have to dispose of the action

This small optimization is useful here because we create one of these actions for every search item in the list as you scroll

For actions that are static, it is better to use `IAction` instead of `Action`. This avoids creating an extra disposable, emitter, and event listener (when used with an action bar). It also means you don't have to dispose of the action

This small optimization is useful here because we create one of these actions for every search item in the list as you scroll
@mjbvz mjbvz added this to the October 2022 milestone Oct 28, 2022
@mjbvz mjbvz requested a review from andreamah October 28, 2022 02:50
@mjbvz mjbvz self-assigned this Oct 28, 2022
@mjbvz mjbvz enabled auto-merge (squash) October 28, 2022 02:50
@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 28, 2022

@andreamah I've only updated this one action since it was the most commonly rendered once. If you agree this is a worthwhile change, you can also apply this same change to the other actions

At the very least, it means you don't have to dispose of the actions that you create, which is easy to forget to do

Copy link
Contributor

@andreamah andreamah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks for finding that!

@mjbvz mjbvz merged commit 3007d54 into microsoft:main Oct 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants